feat: fail fast on implausible commit range in shallow checkout - #49003
Conversation
… path
Add checkImplausibleShallowRange() to git_helpers.cjs that detects when a
shallow clone (fetch-depth:1) causes git rev-list to return the entire local
history as the commit range (e.g. 61k commits for a 1-commit branch). When
the range exceeds SHALLOW_RANGE_MAX_COMMITS (100) and the repo is shallow,
emit an actionable warning/error instead of proceeding to synthesize a
potentially huge/incorrect rewrite commit.
Changes:
- Add SHALLOW_RANGE_MAX_COMMITS = 100 constant
- Add checkImplausibleShallowRange(baseRef, headRef, options) (sync)
that returns { implausible, commitCount } and emits core.warning
- Modify hasMergeCommitsInRange to call the guard first; return false
for implausible ranges to prevent false-positive bundle transport
- Modify linearizeRangeAsCommit to throw before any git state mutation
when range is implausible in a shallow checkout; configurable via
opts.maxCommits; set to Infinity to disable the guard
- Export checkImplausibleShallowRange and SHALLOW_RANGE_MAX_COMMITS
- Add 14 new tests covering both functions and the new guard paths
- Document the fetch-depth workaround in checkout.md and
safe-outputs-pull-requests.md
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage: feat: fail fast on implausible commit range in shallow checkout
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49003 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Adds safeguards intended to detect implausibly large commit ranges in shallow checkouts before rewriting commits.
Changes:
- Adds shallow-range detection and configurable thresholds.
- Guards commit linearization and merge detection.
- Adds tests and checkout guidance.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/git_helpers.cjs |
Adds range guards and linearization checks. |
actions/setup/js/git_helpers.test.cjs |
Tests shallow-range behavior. |
docs/src/content/docs/reference/checkout.md |
Documents checkout-depth implications. |
docs/src/content/docs/reference/safe-outputs-pull-requests.md |
Documents safe-output behavior and remediation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (3)
actions/setup/js/git_helpers.cjs:359
- This guard is unreachable from its only production caller. In
safe_outputs_handlers.cjs:1212-1242, whenpatchFormatExplicitis false the format defaults tobundle(useBundle === true), and whenuseBundleis false anamformat was explicitly supplied (patchFormatExplicit === true), so!useBundle && !patchFormatExplicitcan never hold. Consequently this new shallow-range check never emits its warning or prevents the default bundle path. Invoke the implausible-range guard independently of that impossible auto-switch branch and abort before artifact generation.
const { implausible } = checkImplausibleShallowRange(baseRef, headRef, options);
if (implausible) return false;
actions/setup/js/git_helpers.cjs:320
- None of the added
checkImplausibleShallowRangetests reaches this positive branch: the “non-shallow large range” test uses a one-commit range below the default threshold, and the shallow/large mocks exercise the separate duplicated logic inlinearizeRangeAsCommit. A regression in this helper's shallow probe, warning, or{ implausible: true }result would therefore pass. Add a direct test using a shallow repository with a range above a loweredmaxCommitsthreshold.
if (isShallow) {
core.warning(
`Shallow checkout produced an implausible commit range of ${commitCount} commits for ${baseRef}..${headRef}. ` +
`This usually means ${baseRef} is not reachable from the shallow grafts. ` +
`Increase fetch-depth in your workflow checkout step (e.g. fetch-depth: 0) to resolve this.`
actions/setup/js/git_helpers.cjs:735
- This check runs only after
create_pull_requesthas generated and transferred the patch/bundle, applied the bundle, and attempted the first signed push (create_pull_request.cjs:1681). It therefore cannot satisfy the linked issue’s requirement to detect the implausible range before payload synthesis; the large artifact work has already happened. Invoke the guard in the create-pull-request safe-output handler beforegenerateGitPatch/generateGitBundle, while retaining this pre-mutation defense.
const { stdout: countOut } = await execApi.getExecOutput("git", ["rev-list", "--count", `${baseRef}..HEAD`], ...execArgs);
rangeCommitCount = parseInt(countOut.trim(), 10) || 0;
} catch {
rangeCommitCount = 0;
}
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Medium
| :::caution[Shallow checkout and large monorepos] | ||
| The merge-commit detection that auto-selects bundle transport inspects the commit range `origin/<branch>..<branch>` in the **agent's** workspace. With the default shallow checkout (`fetch-depth: 1`) `origin/<branch>` has no traversable ancestry, so `git rev-list` cannot exclude any commits and will report the entire local history as the range. On large monorepos this produces a count of tens of thousands of commits, which falsely appears to contain merge commits and can trigger an incorrect rewrite. | ||
|
|
||
| The safe_outputs push job guards against this: if the commit range contains more than 100 commits **and** the repository is shallow, the push is refused with a clear error that includes the commit count. To resolve this, increase `fetch-depth` in your workflow checkout step: |
| Fetch everything the workflow needs at checkout time using `fetch-depth` and [`fetch:`](#fetching-additional-refs), and write changes through safe-output tools such as [`push-to-pull-request-branch`](/gh-aw/reference/safe-outputs-pull-requests/) rather than a direct `git push`. The agent is instructed not to configure credential helpers or run `git credential fill`, because authentication cannot succeed; credential errors are reported as a limitation instead of worked around. | ||
|
|
||
| :::note[Shallow checkout and push-to-pull-request-branch] | ||
| `push-to-pull-request-branch` and `create-pull-request` inspect the commit range `origin/<branch>..<branch>` in the agent's workspace to detect merge commits. With the default shallow clone (`fetch-depth: 1`), `origin/<branch>` has no traversable ancestry, so `git rev-list` reports the entire local history as the range. On large monorepos (thousands of commits) this can falsely trigger bundle or rewrite paths and will be refused with a clear error. Set `fetch-depth: 0` to ensure the correct range is visible. |
| * All git failures are treated as non-fatal: errors cause the function to | ||
| * return `{ implausible: false, commitCount: 0 }` so callers are never blocked. |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (14 tests)
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 53.1 AIC · ⌖ 4.85 AIC · ⊞ 5.3K
| `This likely means ${baseRef} is not reachable from the shallow grafts. ` + | ||
| `Increase fetch-depth in your workflow checkout step (e.g. fetch-depth: 0) to resolve this.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
The guard in linearizeRangeAsCommit re-implements shallow-range detection inline rather than reusing checkImplausibleShallowRange. This creates two diverging implementations: the shared helper accepts an explicit headRef and uses execGitSync, while this inline block hardcodes HEAD and uses execApi.getExecOutput. A future change to one path will silently miss the other, and the two already differ (e.g. headRef is not surfaced in the thrown message here vs the shared helper).
Consider adapting checkImplausibleShallowRange to accept an optional execApi so both callers can share a single implementation.
@copilot please address this.
| } catch { | ||
| rangeCommitCount = 0; | ||
| } | ||
| if (Number.isFinite(rangeCommitCount) && rangeCommitCount > maxCommits) { |
There was a problem hiding this comment.
parseInt(...) || 0 already coerces NaN to 0, so rangeCommitCount is always a finite integer after that line. The Number.isFinite(rangeCommitCount) guard on line 736 is therefore always true and adds no protection. The same pattern is in checkImplausibleShallowRange (line ~61). Consider removing the redundant isFinite check to simplify the condition to rangeCommitCount > maxCommits.
@copilot please address this.
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 42 AIC · ⌖ 6.82 AIC · ⊞ 7K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/git_helpers.cjs:168
[/codebase-design] linearizeRangeAsCommit duplicates the shallow-guard logic inline instead of calling checkImplausibleShallowRange, creating two diverging implementations of the same check.
<details>
<summary>💡 Suggested refactor</summary>
checkImplausibleShallowRange is synchronous and uses execGitSync; linearizeRangeAsCommit needs async execApi.getExecOutput. The cleanest fix is to add an async overload or an internal async helper, then replace the inline block with a sing…
actions/setup/js/git_helpers.cjs:56
[/tdd] parseInt(countOut.trim(), 10) || 0 silently treats non-numeric output (e.g. an unexpected git message) as 0, masking parse failures.
<details>
<summary>💡 Suggested fix</summary>
Use a stricter parse that catches NaN explicitly:
const parsed = parseInt(countOut.trim(), 10);
if (Number.isNaN(parsed)) return { implausible: false, commitCount: 0 };
commitCount = parsed;Or use the same pattern already in linearizeRangeAsCommit (line 147) which also uses || 0 — at m…
actions/setup/js/git_helpers.test.cjs:381
[/tdd] There are no tests for hasMergeCommitsInRange with a shallow + large commit range — the function that uses checkImplausibleShallowRange but lacks coverage for that branch.
<details>
<summary>💡 Suggested test</summary>
it("should return false and not call rev-list --merges when range is implausibly large in a shallow checkout", () => {
// mock execGitSync so rev-list --count returns 500 and --is-shallow-repository returns true
// then call hasMergeCommitsInRange and a…
</details>
|
@copilot please run the Recent reviewer feedback to address:
Run: https://github.com/github/gh-aw/actions/runs/30520508562
|
…rage - Fix JSDoc in checkImplausibleShallowRange to accurately document both failure cases: rev-list failure returns commitCount:0; rev-parse failure returns the measured commitCount (not 0) - Fix test to exercise the --is-shallow-repository code path by using maxCommits:0 to force the threshold check on a 1-commit range; test is now environment-aware (works in both shallow and full clones) - Add hasMergeCommitsInRange test coverage: empty refs, non-existent refs, empty range, and environment-aware threshold test - Fix safe-outputs-pull-requests.md to accurately describe behavior: hasMergeCommitsInRange emits warning and returns false (not 'push refused'); only linearizeRangeAsCommit throws on implausible ranges - Fix checkout.md to remove inaccurate mention of create-pull-request inspecting the merge-commit range; only push-to-pull-request-branch uses hasMergeCommitsInRange for transport selection Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all unresolved review feedback in commit
|
Triage Summary
Prevents incorrect commit-range rewrites on large monorepos with shallow checkouts. Has tests+docs. Batch B-git-checkout (with #49002, #49059).
|
|
@copilot add git integration tests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…MergeCommitsInRange, and linearizeRangeAsCommit Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added in commit The new
Two implementation notes: |
…in CI shallow clone Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit Root cause: the CI job uses Fix: added an explicit |
In a shallow clone (
fetch-depth: 1),origin/<base>has no traversable ancestry, sogit rev-list base..HEADreturns the entire local history. On large monorepos this produces counts in the tens-of-thousands, which falsely trips merge-commit detection and forceslinearizeRangeAsCommit— producing incorrect rewrites.Changes
git_helpers.cjsSHALLOW_RANGE_MAX_COMMITS = 100constant (override viamaxCommitsoption)checkImplausibleShallowRange(baseRef, headRef, options)— counts commits in range, checks--is-shallow-repository; if both exceed threshold and are shallow, emitscore.warningwith an actionable message; all git failures non-fatalhasMergeCommitsInRange— calls the guard first; returnsfalsefor implausible ranges, preventing false-positive bundle-transport switchinglinearizeRangeAsCommit— adds async guard before any git state mutation (before soft-reset); throws with count, base ref, andfetch-depthfix when implausible; configurable viaopts.maxCommits(setInfinityto disable)git_helpers.test.cjs— 14 new tests covering both functions: empty/invalid refs, git failure paths, non-shallow large ranges, shallow large ranges, custom threshold, rev-list/shallow-probe failure non-fatalityDocs — Added caution blocks in
checkout.mdandsafe-outputs-pull-requests.mddocumenting the shallow-range limitation andfetch-depth: 0workaroundError behavior
When a shallow checkout produces an implausible range,
linearizeRangeAsCommitnow throws before touching the index:And
hasMergeCommitsInRangeemits a warning and returnsfalse(no bundle-force) for the same condition.